tabletserver: improved state transitions with new stateManager by sougou · Pull Request #6396 · vitessio/vitess

sougou · 2020-06-30T02:39:55Z

The tabletserver can now remember its desired state and continuously retry if the first attempt fails. This responsibility was previously with tabletmanager. This change allows us to delete the corresponding code from tabletmanager.

Most sub-component function calls have been normalized to simplify state management. All such functions are now idempotent.

The new stateManager invokes the necessary functions to reach any required state knowing that the functions are idempotent. This allows it to act in a manner that is agnostic of what the current state is.

Strict ordering rules have been defined for how to open and close various services.

systay · 2020-06-30T14:09:31Z

go/vt/dbconfigs/dbconfigs.go

vterrors FTW

systay · 2020-06-30T14:13:59Z

go/vt/vttablet/tabletserver/state_manager.go

"golang.org/x/net/context" -> "context"

systay · 2020-06-30T14:15:11Z

go/vt/vttablet/tabletserver/state_manager.go

It would be really nice if states are not just plain int64, but their own type, built on int64. It helps understanding and gives compiler protection for a bunch of situations.

go/vt/vttablet/tabletserver/state_manager.go

systay · 2020-06-30T14:25:43Z

go/vt/vttablet/tabletserver/state_manager_test.go

super-nit: I think it would be nice if we changed the order of these, so that order comes before component. That way, assertions line up better:

verifySubcomponent(t, 1, sm.messager, testStateClosed) verifySubcomponent(t, 2, sm.te, testStateClosed) verifySubcomponent(t, 3, sm.txThrottler, testStateClosed) verifySubcomponent(t, 4, sm.qe, testStateClosed) verifySubcomponent(t, 5, sm.watcher, testStateClosed) ...

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

Tests still need fixing Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

Looks like tm explicitly sets it expecting tablet server to unset it. We'll need to revisit this. Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

Also address a few early comments. Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

If the requested type is RESTORE, force StateNotConnected. Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

Some tests became flaky because they expect the state to change immediately after a call. This change brings the behavior to be closer to the existing one. But the tabletserver will continue to retry if an request failed. Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

harshit-gangal

Overall looks good. Some nits.

harshit-gangal · 2020-07-12T18:07:40Z

go/vt/vttablet/tabletserver/vstreamer/engine.go

+// InitDBConfig initializes the target parameters for the Engine.
+func (vse *Engine) InitDBConfig(keyspace string) {
+	vse.keyspace = keyspace
+}
+


nit: this can be renamed to setKeyspace as it is only setting keyspace.

harshit-gangal · 2020-07-12T18:36:57Z

go/vt/vttablet/tabletserver/txthrottler/tx_throttler.go

+// InitDBConfig initializes the target parameters for the throttler.
+func (t *TxThrottler) InitDBConfig(target querypb.Target) {
+	t.target = target
+}
+


nit: InitDBConfig -> setTarget

harshit-gangal · 2020-07-12T19:06:20Z

go/vt/vttablet/tabletserver/query_engine.go

 // Open must be called before sending requests to QueryEngine.
 func (qe *QueryEngine) Open() error {
+	if qe.isOpen {
+		return nil
+	}


Should this check be not performed under lock or is it protected by higher order?

The state manager ensures that Open and Close are not called concurrently. A lock is needed only if the variable is accessed by other functions, which is the case in the other situations.

There are a few places where I do this check inside a lock even though it's not needed. No harm.

go/vt/vttablet/tabletserver/state_manager.go

harshit-gangal · 2020-07-12T19:31:44Z

go/vt/vttablet/tabletserver/state_manager.go

+		case <-tmr.C:
+			log.Fatal("Shutdown took too long. Crashing")


test for this?

This can't be tested because it will crash the test :).

sougou

The InitDBConfig is a naming convention that implies the completion of an incomplete "New" instantiation. So, giving it other meaningful names will cause this semantic to get lost.

I'm hoping to get rid of this two-phase initialization as part of my refactor. Let's see if I succeed.

sougou · 2020-07-14T00:32:23Z

go/vt/vttablet/tabletserver/query_engine.go

 // Open must be called before sending requests to QueryEngine.
 func (qe *QueryEngine) Open() error {
+	if qe.isOpen {
+		return nil
+	}


The state manager ensures that Open and Close are not called concurrently. A lock is needed only if the variable is accessed by other functions, which is the case in the other situations.

There are a few places where I do this check inside a lock even though it's not needed. No harm.

sougou · 2020-07-14T00:33:31Z

go/vt/vttablet/tabletserver/state_manager.go

+		case <-tmr.C:
+			log.Fatal("Shutdown took too long. Crashing")


This can't be tested because it will crash the test :).

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

sougou requested review from deepthi, harshit-gangal and systay June 30, 2020 02:40

systay reviewed Jun 30, 2020

View reviewed changes

go/vt/dbconfigs/dbconfigs.go Outdated

Copy link
Copy Markdown

Collaborator

systay Jun 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

vterrors FTW

systay reviewed Jun 30, 2020

View reviewed changes

go/vt/vttablet/tabletserver/state_manager.go Outdated

Copy link
Copy Markdown

Collaborator

systay Jun 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"golang.org/x/net/context" -> "context"

systay reviewed Jun 30, 2020

View reviewed changes

go/vt/vttablet/tabletserver/state_manager.go Outdated Show resolved Hide resolved

systay reviewed Jun 30, 2020

View reviewed changes

sougou force-pushed the ss-ts1-state branch 2 times, most recently from c42ab86 to c6d295f Compare July 3, 2020 01:06

sougou added 18 commits July 11, 2020 14:01

vttablet: fail if schema engine cannot open

c6f64d0

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

vttablet: standardize heartbeat initialization

7d8eccb

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

vttablet: standardize query_engine

7881fe1

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

vttablet: standardize txThrottler initialization

eb93c73

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

vttablet: standardize tx_engine initialization

caae5ee

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

vttablet: standardize vstreamer initialization

7c0b7f6

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

vttablet: further standardize heartbeat

0491e9b

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

vttablet: move functions to state_manager.go

be69987

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

vttablet: componentize states into stateManager

92905e3

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

vttablet: implement new state transitions

35f65b0

Tests still need fixing Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

vttablet: stateManager tests WIP

3efba4b

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

vttablet: tests for stateManager

919ad40

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

vttablet: re-introduce ExitLameduck

40ac47b

Looks like tm explicitly sets it expecting tablet server to unset it. We'll need to revisit this. Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

vttablet: detect shutindown state in StartRequest

f25002e

Also address a few early comments. Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

vttablet: special-case RESTORE type

9691dea

If the requested type is RESTORE, force StateNotConnected. Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

vttablet: address review comments

e23cf2a

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

fix golangci errors

dbc50a0

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

sougou force-pushed the ss-ts1-state branch from c6d295f to dbc50a0 Compare July 11, 2020 21:24

harshit-gangal approved these changes Jul 12, 2020

View reviewed changes

sougou commented Jul 14, 2020

View reviewed changes

vttablet: address review comments

5ebab3a

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

sougou merged commit b6c7ffc into vitessio:master Jul 14, 2020

sougou deleted the ss-ts1-state branch July 14, 2020 02:03

deepthi added this to the v7.0 milestone Jul 17, 2020

ameetkotian mentioned this pull request Aug 20, 2020

Slack vitess 2020.08.19.r0 tinyspeck/vitess#180

Merged

GuptaManan100 mentioned this pull request Dec 6, 2022

Tracking CheckMySQL and Query Engine history #11885

Closed

Conversation

sougou commented Jun 30, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

systay Jun 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

harshit-gangal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sougou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

systay Jun 30, 2020 •

edited

Loading